Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r1 (raw file):

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.

Suggestion:

TODO(Stav)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

#[derive(Debug)]
pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: MemorySegmentAddresses,

struct represents only buildtins, no need to mention that

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r1 (raw file):

}

impl Default for BuiltinsSegments {

please remove that

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 136 at r1 (raw file):

                    }
                }
                // Output, executaion and program segments are not builtins.

typo

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 144 at r1 (raw file):

        }
        res
    }

better?

Suggestion:

pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: Option<MemorySegmentAddresses>,
    pub range_check_bits_96_builtin: Option<MemorySegmentAddresses>,
    pub bitwise_builtin: Option<MemorySegmentAddresses>,
    pub add_mod_builtin: Option<MemorySegmentAddresses>,
    pub ec_op_builtin: Option<MemorySegmentAddresses>,
    pub ecdsa_builtin: Option<MemorySegmentAddresses>,
    pub keccak_builtin: Option<MemorySegmentAddresses>,
    pub mul_mod_builtin: Option<MemorySegmentAddresses>,
    pub pedersen_builtin: Option<MemorySegmentAddresses>,
    pub poseidon_builtin: Option<MemorySegmentAddresses>,
}
impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,
    ) -> Self {
        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            match *name {
                "range_check" => {
                    res.range_check_bits_128_builtin =
                        Some((value.begin_addr, value.stop_ptr).into())
                }

                "range_check96" => {
                    res.range_check_bits_96_builtin =
                        Some((value.begin_addr, value.stop_ptr).into())
                }
                "bitwise" => res.bitwise_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "add_mod" => res.add_mod_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "ec_op" => res.ec_op_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "ecdsa" => res.ecdsa_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "keccak" => res.keccak_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "mul_mod" => res.mul_mod_builtin = Some((value.begin_addr, value.stop_ptr).into()),
                "pedersen" => {
                    res.pedersen_builtin = Some((value.begin_addr, value.stop_ptr).into())
                }
                "poseidon" => {
                    res.poseidon_builtin = Some((value.begin_addr, value.stop_ptr).into())
                }

                // Output, executaion and program segments are not builtins.
                "output" => {}
                "execution" => {}
                "program" => {}
                _ => panic!("Unknown memory segment: {name}"),
            }
        }
        res
    }
}

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 217 at r1 (raw file):

        assert_eq!(components.mul_opcode_is_small_t_is_imm_t.len(), 0);
        assert_eq!(components.mul_opcode_is_small_t_is_imm_f.len(), 0);
        assert_eq!(components.mul_opcode_is_small_f_is_imm_f.len(), 11146);

why are these changed? are you running with dev mode on?
turn it off

Code quote:

l

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 300 at r1 (raw file):

        let input = small_cairo_input();

        // Check opcode components.

Suggestion:

Test

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 359 at r1 (raw file):

        assert_eq!(components.ret_opcode.len(), 462);

        // Check builtins

same and point at eol.

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 360 at r1 (raw file):

        // Check builtins
        let builtins = input.builtins_segments;

Suggestion:

builtins_segments

stwo_cairo_prover/crates/prover/src/input/plain.rs line 73 at r1 (raw file):

            })
        });
    let trace = runner.relocated_trace.clone().unwrap();

it's a large object
its possible to reorder the code and avoid this

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 219 at r1 (raw file):

    }

    // When not ignored, the test passes only with dev_mod = false.

is this related?

Reviewable status: 0 of 4 files reviewed, 11 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 145 at r1 (raw file):

        res
    }
}

better!

Suggestion:

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.
/// This struct holds the builtins used in a Cairo program.
/// Most of them are not implemented yet by Stwo.
#[derive(Debug, Default)]
pub struct BuiltinsSegments {
    pub range_check_bits_128_builtin: Option<MemorySegmentAddresses>,
    pub range_check_bits_96_builtin: Option<MemorySegmentAddresses>,
    pub bitwise_builtin: Option<MemorySegmentAddresses>,
    pub add_mod_builtin: Option<MemorySegmentAddresses>,
    pub ec_op_builtin: Option<MemorySegmentAddresses>,
    pub ecdsa_builtin: Option<MemorySegmentAddresses>,
    pub keccak_builtin: Option<MemorySegmentAddresses>,
    pub mul_mod_builtin: Option<MemorySegmentAddresses>,
    pub pedersen_builtin: Option<MemorySegmentAddresses>,
    pub poseidon_builtin: Option<MemorySegmentAddresses>,
}
impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,
    ) -> Self {
        let mut res = BuiltinsSegments::default();
        for (name, value) in memory_segments.iter() {
            let value = (value.begin_addr, value.stop_ptr).into();
            match *name {
                "range_check" => res.range_check_bits_128_builtin = Some(value),
                "range_check96" => res.range_check_bits_96_builtin = Some(value),
                "bitwise" => res.bitwise_builtin = Some(value),
                "add_mod" => res.add_mod_builtin = Some(value),
                "ec_op" => res.ec_op_builtin = Some(value),
                "ecdsa" => res.ecdsa_builtin = Some(value),
                "keccak" => res.keccak_builtin = Some(value),
                "mul_mod" => res.mul_mod_builtin = Some(value),
                "pedersen" => res.pedersen_builtin = Some(value),
                "poseidon" => res.poseidon_builtin = Some(value),
                // Output, executaion and program segments are not builtins.
                "output" => {}
                "execution" => {}
                "program" => {}
                _ => panic!("Unknown memory segment: {name}"),
            }
        }
        res
    }
}

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

It is because the name of the component in the air-infra ends with"builtin" and in the future we would like to make sure it sync.(Same for the _opcdoe in CasmStatesByOpcodes)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 22 at r1 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 145 at r1 (raw file):

Much better! Thanks

stwo_cairo_prover/crates/prover/src/input/plain.rs line 73 at r1 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 217 at r1 (raw file):

I did s(ame as 245)

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 219 at r1 (raw file):

Not relevant anymore

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 359 at r1 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r1 (raw file):

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(STAV): Understand if the adapter should pass builtins that won't be supported by stwo.

Done.

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 300 at r1 (raw file):

        let input = small_cairo_input();

        // Check opcode components.

Done.

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 360 at r1 (raw file):

        // Check builtins
        let builtins = input.builtins_segments;

Done.

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

im not sure it should appear there, @DavidLevitGurevich wdyt?

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 29 at r2 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 41 at r2 (raw file):

                "pedersen" => res.pedersen_builtin = Some(value),
                "poseidon" => res.poseidon_builtin = Some(value),
                // Output, execution and program segments are not builtins.

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 44 at r2 (raw file):

                "output" => {}
                "execution" => {}
                "program" => {}

Done.

Reviewed all commit messages.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @Stavbe)

stwo_cairo_prover/crates/adapted_prover/src/main.rs line 68 at r3 (raw file):

    let args = Args::try_parse_from(args)?;

    // dev_mode = true

remove

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @DavidLevitGurevich and @ohad-starkware)

stwo_cairo_prover/crates/adapted_prover/src/main.rs line 68 at r3 (raw file):

Done.

Reviewed 1 of 4 files at r1, 1 of 4 files at r2, 1 of 2 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @Stavbe)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r4 (raw file):

use cairo_vm::air_public_input::MemorySegmentAddresses;

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.

I think we can decide on that now and remove the TODO.
Any reason not to support all the builtins in this struct?

Code quote:

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 7 at r4 (raw file):

// TODO(Stav): Understand if the adapter should pass builtins that won't be supported by stwo.
/// This struct holds the builtins used in a Cairo program.
/// Most of them are not implemented yet by Stwo.

Remove

Code quote:

/// Most of them are not implemented yet by Stwo.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 9 at r4 (raw file):

/// Most of them are not implemented yet by Stwo.
#[derive(Debug, Default)]
pub struct BuiltinsSegments {

Does Default here set all the values as None?
I think the code that construct this struct should have a unit test

Code quote:

#[derive(Debug, Default)]
pub struct BuiltinsSegments {

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 24 at r4 (raw file):

impl BuiltinsSegments {
    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(

Consider to implement the From trait, and then you'll be able to just use into()
non-blocking

Code quote:

    pub fn from_memory_segments(

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 25 at r4 (raw file):

    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,

Suggestion:

    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, MemorySegmentAddresses>,

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 31 at r4 (raw file):

            let value = Some((value.begin_addr, value.stop_ptr).into());
            match *name {
                "range_check" => res.range_check_bits_128_builtin = value,

Consider to use BuitinName enum from cairo_vm instead of hardcoded strings

Code quote:

            match *name {
                "range_check" => res.range_check_bits_128_builtin = value,

Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @shaharsamocha7)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r4 (raw file):

If Stwo won't do anything with then why?

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 7 at r4 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 9 at r4 (raw file):

Yes, added.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 25 at r4 (raw file):

    /// Create a new `BuiltinsSegments` struct from a map of memory MemorySegmentAddressess.
    pub fn from_memory_segments(
        memory_segments: &HashMap<&str, cairo_vm::air_public_input::MemorySegmentAddresses>,

Done.

Reviewed 3 of 4 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, and @Stavbe)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r4 (raw file):

I don't understand, stwo has to handle all the builtins that are used by the program.
We can't just ignore them.

Another alternative is to assert that the "unsupported" builtins are not in use
but I don't think this is a good place for this.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 25 at r4 (raw file):

I'm sorry but the previous impl was better because I don't like the
let builtins_segments = memory_segments.into();

Can you revert?

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 73 at r5 (raw file):

        assert_eq!(builtins_segments.add_mod_builtin, None);
        assert_eq!(builtins_segments.ec_op_builtin, None);
        assert_eq!(builtins_segments.ecdsa_builtin, Some((353, 353).into()));

Is this a correct behaviour?
I think that if the range is 0 we want to set it as None.
@yuvalsw WDYT?

Code quote:

assert_eq!(builtins_segments.ecdsa_builtin, Some((353, 353).into()));

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line 425 at r5 (raw file):

  ],
  "dynamic_params": null
}

Add new line

Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @DavidLevitGurevich, @ohad-starkware, @shaharsamocha7, and @yuvalsw)

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 10 at r1 (raw file):

Since there are builtins here that won’t be implemented by the air-infra, it’s not relevant. Deleted.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 5 at r4 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 25 at r4 (raw file):

Done.

stwo_cairo_prover/crates/prover/src/input/builtins_segments.rs line 73 at r5 (raw file):

Any idea what the difference is between builtins that don’t get a memory segment and those whose range is 0?

stwo_cairo_prover/crates/prover/src/input/test_builtins_segments/air_pub.json line 425 at r5 (raw file):

Done.